feat: warn if define['process.env'] contains path key with a value#19517
Conversation
sapphi-red
added
the
p2-nice-to-have
I'd prefer to keep it as-is to reduce the false positives. |
|
Hey @sapphi-red, Good job, i like your approach too 🤝 |
|
@sapphi-red While I believe this is a step in the right direction but I do believe that changing the doc would be more beneficial, even with a warning some people will still ignore it or don't read it. The best way to save those people is to not have a problematic code that they can copy and paste. I believe that @qu35t-code suggestion provides a safer snippet that will not be problematic for users that don't read the documentation or the warning and would sill allow conscious developer to achieve what they want if required. |
|
@Techbrunch I replied at #19510 (comment) to keep this PR focused on the warning. |
bluwy
left a comment
bluwy
left a comment
There was a problem hiding this comment.
Leaving an approval for now and we can tweak the heuristic or warning again if needed.
patak-cat
left a comment
patak-cat
left a comment
There was a problem hiding this comment.
Nice one sapphi, I also think a warning will help more users here
5 participants
Description
Added a warning if
define['process.env']containspath: "some value".There are a lot of people passing
'process.env': process.env(search). This shouldn't be done because it may expose all the env vars, which may contain access tokens.There were some PRs / issues that suggests to update the docs (#16686, #18441, #19510), but I don't think that can prevent people from doing it because the reason they did was because they didn't read the docs.
This PR adds a warning so that they can notice even without reading the docs.
The warning is output based on the following assumption:
Path/PATH/pathis declared on the machine and has a non-empty value: I believe this holds true in most casesprocess.env.pathis not expected to be replaced: in this case the added warning will be output even if they only havepath: "intended-value". But I don't think this will likely to happen and they can workaround by setting'process.env.path': 'value'instead of'process.env': { path: 'value' }.close #18441
close #19510